-
Notifications
You must be signed in to change notification settings - Fork 2k
[None][chore] Spark ci tests #9920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis pull request adds GB10x GPU support to the Jenkins test pipeline and test infrastructure. Changes include Kubernetes pod configuration updates with GB10x-specific cloud and memory settings, conditional device handling for GPU node clusters, and fallback mechanisms for GPU memory detection when pynvml is unavailable. Test configurations are expanded to include GB10 PyTorch tests with pre- and post-merge variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f474920 to
ae68954
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/defs/conftest.py (1)
2683-2752: Improve logging and exception handling in GPU memory fallback pathThe new NVML-less fallback and the wrapped
collect_statuslogic are a good robustness improvement, but a couple of details could be tightened:
- There are multiple
except Exception: passblocks (e.g., aroundfree -m, per‑process psutil calls, and in the NVML detail loop). These can silently hide real issues; consider at least logging at debug/warning level or narrowing to the expected exceptions (CalledProcessError,ValueError,IndexError, etc.) so you still get signal when the fallback is misbehaving.- The outer
except Exceptionaround the NVML path causes any NVML‑side issue (including “no GPUs” or unexpected NVML errors) to print “pynvml not available, using fallback…”, which can be misleading. If you care about diagnosability, you might split this into a smallertrythat only covers import/init and NVML queries, and either log the actual exception type/message or adjust the fallback warning text.- For consistency with the rest of this file, you may want to replace raw
print(...)withprint_warning/print_info(or a logger) so Jenkins logs stay uniform.These don’t affect correctness but will make CI debugging easier when the fallback kicks in.
Also applies to: 2763-2815
jenkins/L0_Test.groovy (1)
1358-1375: Review of gb10x Kubernetes pod config: cloud, selectors, tolerations, and GPU envThe gb10x‑specific branch in
createKubernetesPodConfiglooks consistent overall:
targetCloud = "nvks-sparks-cloud"plus the DGX Spark selectors and thenode_for_blossom_trttoleration clearly route these jobs to the intended SBSA Spark pool.- Memory and storage sizing (64Gi for gb10x) and inclusion in
REQUIRED_NO_DRIVER_TYPESfit the “no dynamic driver flashing” intent.One thing to double‑check operationally: setting
env: - name: NVIDIA_VISIBLE_DEVICES value: "all" - name: NVIDIA_DRIVER_CAPABILITIES value: "compute,utility"on the test container for all GPU types may interact with how your cluster’s NVIDIA device plugin or runtime sets
NVIDIA_VISIBLE_DEVICESfor resource‑isolated pods (especially under NVKS / MIG). If the plugin also injects this env var, you’ll want to confirm which value wins and that you’re not accidentally exposing more GPUs than requested.If this override is required for nvks-sparks-cloud, consider scoping it to those clouds/types only, or documenting the expectation near this code.
Also applies to: 1455-1473, 1561-1617
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jenkins/L0_Test.groovy(10 hunks)tests/integration/defs/conftest.py(1 hunks)tests/integration/defs/sysinfo/get_sysinfo.py(1 hunks)tests/integration/test_lists/test-db/l0_gb10.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+
Indent Python code with 4 spaces; do not use tabs
Always maintain the namespace when importing in Python, even if only one class or function from a module is used (e.g., usefrom package.subpackage import fooand thenfoo.SomeClass()instead offrom package.subpackage.foo import SomeClass)
Python filenames should use snake_case (e.g.,some_file.py)
Python class names should use PascalCase (e.g.,class SomeClass)
Python function and method names should use snake_case (e.g.,def my_awesome_function():)
Python local variable names should use snake_case, with prefixkfor variable names that start with a number (e.g.,k_99th_percentile = ...)
Python global variables should use upper snake_case with prefixG(e.g.,G_MY_GLOBAL = ...)
Python constants should use upper snake_case (e.g.,MY_CONSTANT = ...)
Avoid shadowing variables declared in an outer scope in Python
Initialize all externally visible members of a Python class in the constructor
For Python interfaces that may be used outside a file, prefer docstrings over comments
Python comments should be reserved for code within a function, or interfaces that are local to a file
Use Google style docstrings for Python classes and functions, which can be parsed by Sphinx
Python attributes and variables can be documented inline with type and description (e.g.,self.x = 5followed by"""<type>: Description of 'x'""")
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except clause to the smallest set of specific errors possible instead of catching all exceptions
When using try-except blocks in Python to handle multiple possible variable types (duck-typing), keep the body of the try as small as possible and use the else block to implement the logic
Files:
tests/integration/defs/sysinfo/get_sysinfo.pytests/integration/defs/conftest.py
**/*.{cpp,h,cu,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code files should contain an NVIDIA copyright header that includes the current year at the top
Files:
tests/integration/defs/sysinfo/get_sysinfo.pytests/integration/defs/conftest.py
🧠 Learnings (4)
📚 Learning: 2025-08-26T09:49:04.956Z
Learnt from: pengbowang-nv
Repo: NVIDIA/TensorRT-LLM PR: 7192
File: tests/integration/test_lists/test-db/l0_dgx_b200.yml:56-72
Timestamp: 2025-08-26T09:49:04.956Z
Learning: In TensorRT-LLM test configuration files, the test scheduling system handles wildcard matching with special rules that prevent duplicate test execution even when the same tests appear in multiple yaml files with overlapping GPU wildcards (e.g., "*b200*" and "*gb200*").
Applied to files:
tests/integration/test_lists/test-db/l0_gb10.ymljenkins/L0_Test.groovy
📚 Learning: 2025-09-09T09:40:45.658Z
Learnt from: fredricz-20070104
Repo: NVIDIA/TensorRT-LLM PR: 7645
File: tests/integration/test_lists/qa/llm_function_core.txt:648-648
Timestamp: 2025-09-09T09:40:45.658Z
Learning: In TensorRT-LLM test lists, it's common and intentional for the same test to appear in multiple test list files when they serve different purposes (e.g., llm_function_core.txt for comprehensive core functionality testing and llm_function_core_sanity.txt for quick sanity checks). This duplication allows tests to be run in different testing contexts.
Applied to files:
tests/integration/test_lists/test-db/l0_gb10.yml
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
Repo: NVIDIA/TensorRT-LLM PR: 6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
tests/integration/test_lists/test-db/l0_gb10.yml
📚 Learning: 2025-08-06T13:58:07.506Z
Learnt from: galagam
Repo: NVIDIA/TensorRT-LLM PR: 6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
Applied to files:
tests/integration/test_lists/test-db/l0_gb10.yml
🪛 Ruff (0.14.8)
tests/integration/defs/conftest.py
2687-2687: f-string without any placeholders
Remove extraneous f prefix
(F541)
2694-2694: Function call with shell=True parameter identified, security issue
(S604)
2699-2700: try-except-pass detected, consider logging the exception
(S110)
2699-2699: Do not catch blind exception: Exception
(BLE001)
2704-2704: Function call with shell=True parameter identified, security issue
(S604)
2738-2739: try-except-pass detected, consider logging the exception
(S110)
2738-2738: Do not catch blind exception: Exception
(BLE001)
2742-2742: Do not catch blind exception: Exception
(BLE001)
2785-2785: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
2805-2806: try-except-pass detected, consider logging the exception
(S110)
2805-2805: Do not catch blind exception: Exception
(BLE001)
2813-2813: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (3)
tests/integration/test_lists/test-db/l0_gb10.yml (1)
1-40: GB10 test matrix wiring looks consistent with Jenkins stagesThe two entries (pre_merge/post_merge) use the expected GB10 constraints (1 GPU,
*gb10*, ubuntu+aarch64, backend=pytorch), and the file name/stage terms line up with the new Jenkins stagesGB10-PyTorch-1andGB10-PyTorch-Post-Merge-1. This should integrate cleanly with the existing test-db/mako flow. Based on learnings, having dedicated GB10 lists alongside other GPU wildcards is fine and won’t cause duplicate scheduling.jenkins/L0_Test.groovy (2)
673-677: Good safeguard around/dev/gdrdrvdevice mountConditionally appending
--device=/dev/gdrdrv:/dev/gdrdrvonly when the node actually has/dev/gdrdrvavoids spurious container startup failures on clusters where GDR is absent, while preserving behavior ondlclusternodes that do have it.
3003-3015: GB10 SBSA stage wiring matches new test-db and gb10x platformThe added SBSA test configs:
SBSATestConfigs = [ "GH200-TensorRT-Post-Merge-1": ["gh200", "l0_gh200", 1, 1], "GB10-PyTorch-1": ["gb10x", "l0_gb10", 1, 1], ] ... SBSASlurmTestConfigs = [ ... "GB10-PyTorch-Post-Merge-1": ["gb10x", "l0_gb10", 1, 1], ]plus extending
REQUIRED_NO_DRIVER_TYPESwith"gb10x"tie together cleanly:
- Stage names follow the existing convention (
<GPU>-<Backend>[-Post-Merge]-<shard>), sogetMakoArgsFromStageNamewill derivestage=pre_merge/post_mergeandbackend=pytorchcorrectly.- The
gb10xplatform string aligns with the newl0_gb10.ymlentry and the gb10x branch increateKubernetesPodConfig, so these stages will use the Spark cloud path you introduced.I don’t see structural issues with this wiring; it should give you the intended GB10 pre‑ and post‑merge coverage.
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #27903 [ run ] triggered by Bot. Commit: |
|
PR_Github #27903 [ run ] completed with state |
|
/bot run --stage-list "GB10-PyTorch-1" |
|
PR_Github #28428 [ run ] triggered by Bot. Commit: |
|
PR_Github #28428 [ run ] completed with state |
|
/bot run --stage-list "GB10-PyTorch-1" |
|
PR_Github #28447 [ run ] triggered by Bot. Commit: |
|
PR_Github #28447 [ run ] completed with state |
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #28469 [ run ] triggered by Bot. Commit: |
|
PR_Github #28469 [ run ] completed with state |
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #29174 [ run ] triggered by Bot. Commit: |
|
PR_Github #29174 [ run ] completed with state
|
|
/bot run --stage-list "GB10-PyTorch-Post-Merge-1" --disable-fail-fast |
|
PR_Github #29279 [ run ] triggered by Bot. Commit: |
|
PR_Github #29279 [ run ] completed with state
|
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #29453 [ run ] triggered by Bot. Commit: |
|
PR_Github #29453 [ run ] completed with state
|
|
PR_Github #29666 [ run ] triggered by Bot. Commit: |
|
PR_Github #29667 [ run ] triggered by Bot. Commit: |
|
PR_Github #29667 [ run ] completed with state
|
6bdad5e to
797d95e
Compare
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #30240 [ run ] triggered by Bot. Commit: |
|
PR_Github #30240 [ run ] completed with state
|
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #30306 [ run ] triggered by Bot. Commit: |
|
PR_Github #30306 [ run ] completed with state
|
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #30398 [ run ] triggered by Bot. Commit: |
|
PR_Github #30398 [ run ] completed with state
|
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #30426 [ run ] triggered by Bot. Commit: |
|
PR_Github #30426 [ run ] completed with state
|
797d95e to
55ac8db
Compare
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #30778 [ run ] triggered by Bot. Commit: |
|
PR_Github #30779 [ run ] triggered by Bot. Commit: |
|
PR_Github #30778 [ run ] completed with state |
|
PR_Github #30779 [ run ] completed with state
|
Signed-off-by: Jenny Liu <[email protected]>
Signed-off-by: list <[email protected]>
55ac8db to
3fb19de
Compare
|
/bot run --stage-list "GB10-PyTorch-1" --disable-fail-fast |
|
PR_Github #30946 [ run ] triggered by Bot. Commit: |
|
PR_Github #30946 [ run ] completed with state
|
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
Details
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.